Files that changed from the base of the PR and between 9bed76d and df9a74d.

• flake.nix (1 hunks)
• hscontrol/app.go (5 hunks)
• hscontrol/auth.go (11 hunks)
• hscontrol/db/db.go (3 hunks)
• hscontrol/db/node.go (4 hunks)
• hscontrol/db/preauth_keys.go (3 hunks)
• hscontrol/db/routes.go (1 hunks)
• hscontrol/db/users.go (6 hunks)
• hscontrol/db/users_test.go (2 hunks)
• hscontrol/grpcv1.go (3 hunks)
• hscontrol/handlers.go (3 hunks)
• hscontrol/mapper/mapper.go (3 hunks)
• hscontrol/mapper/mapper_test.go (4 hunks)
• hscontrol/mapper/tail.go (1 hunks)
• hscontrol/oidc.go (11 hunks)
• hscontrol/policy/acls.go (3 hunks)
• hscontrol/policy/acls_test.go (3 hunks)
• hscontrol/suite_test.go (1 hunks)
• hscontrol/types/config.go (13 hunks)
• hscontrol/types/config_test.go (4 hunks)
• hscontrol/types/node.go (2 hunks)
• hscontrol/types/node_test.go (4 hunks)
• hscontrol/types/users.go (5 hunks)
• hscontrol/util/dns.go (2 hunks)
• hscontrol/util/dns_test.go (1 hunks)
• integration/auth_oidc_test.go (3 hunks)

• hscontrol/types/config_test.go

hscontrol/suite_test.go (1)

49-49: Verify the default behaviour of OIDCConfig.

The OIDC field is now initialized with an empty configuration. Ensure that the default values, especially for StripEmaildomain, align with the application's requirements.

hscontrol/db/users_test.go (1)

23-23: Verify consistent usage of GetUserByName.

The method GetUser has been renamed to GetUserByName. Ensure that this change is consistently applied throughout the codebase.

Also applies to: 76-76, 79-79

Verification successful

---

Consistent Usage of GetUserByName Verified

The method GetUserByName is used consistently across the codebase, with no instances of the old method name GetUser found. The renaming has been applied correctly in all relevant files.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent usage of the GetUserByName method across the codebase.

# Test: Search for GetUserByName method usage. Expect: Consistent usage throughout the codebase.
rg --type go 'GetUserByName'

Length of output: 1278

hscontrol/mapper/tail.go (1)

79-79: Verify the correctness of GetFQDN with reduced parameters.

The cfg parameter has been removed from the GetFQDN function call. Ensure that the function operates correctly with only cfg.BaseDomain.

Verification successful

---

The GetFQDN function operates correctly with the reduced parameter set. The function is defined to accept a single string parameter, which matches the usage in tail.go. No issues were found with this change.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correctness of GetFQDN with the reduced parameter set.

# Test: Search for the GetFQDN function definition. Expect: Confirm it operates correctly with the new parameter set.
rg --type go 'func GetFQDN' -A 5

Length of output: 34

---

Script:

#!/bin/bash
# Use ast-grep to search for the GetFQDN function definition and its parameters.
ast-grep --lang go --pattern 'func GetFQDN($_) { $$$ }'

Length of output: 57

---

Script:

#!/bin/bash
# Use rg to search for any mention of GetFQDN in the codebase.
rg 'GetFQDN' --type go -A 10

Length of output: 2345

hscontrol/types/users.go (5)

21-43: Enhancements to the User struct look good.

The additional fields for OIDC support and user information management are well-integrated.

---

58-59: The DisplayNameOrUsername method is well-implemented.

The method provides a clear fallback mechanism for user identification.

Tools

golangci-lint

58-58: ST1016: methods on the same type should have the same receiver name (seen 1x "n", 7x "u")

(stylecheck)

---

62-64: The profilePicURL method is correctly implemented.

It simply returns the ProfilePicURL field, as expected.

Tools

golangci-lint

62-62: Comment should end in a period

(godot)

---

123-132: The FromClaim method is effectively synchronising user data.

The method correctly updates user fields based on OIDC claims.

---

106-109: The Proto method is correctly implemented.

The method accurately converts a User to its protobuf representation.

flake.nix (1)

34-34: The vendorHash update is appropriate.

This change reflects updates to dependencies or vendor packages.

hscontrol/db/users.go (5)

52-54: The use of GetUserByName improves clarity.

Renaming the function to specify retrieval by name enhances code readability.

---

93-101: The RenameUser function changes are consistent and clear.

The renaming aligns with the new naming convention for user retrieval.

---

118-124: The GetUserByName function is well-implemented.

The function correctly retrieves a user by name and handles errors.

---

136-152: The GetUserByOIDCIdentifier function effectively extends functionality.

This addition enhances user retrieval capabilities using OIDC identifiers.

---

176-178: The ListNodesByUser function changes are consistent and clear.

Using GetUserByName aligns with the naming convention and improves readability.

hscontrol/db/preauth_keys.go (2)

47-49: The CreatePreAuthKey function changes enhance clarity.

Using GetUserByName improves the readability of user retrieval logic.

---

109-111: The ListPreAuthKeys function changes are consistent and clear.

Using GetUserByName aligns with the naming convention and improves readability.

hscontrol/util/dns.go (2)

Line range hint 13-34: Review CheckForFQDNRules for completeness.

The CheckForFQDNRules function checks for FQDN compliance but does not normalize inputs. Ensure that inputs are pre-normalized or that the function's limitations are documented.

---

Line range hint 1-1: Verify impact of removed normalization functions.

The removal of NormalizeToFQDNRulesConfigFromViper and NormalizeToFQDNRules may affect parts of the codebase that expected user names to be normalized. Ensure that all dependencies on these functions are updated accordingly.

Verification successful

---

No Impact from Removed Normalisation Functions

The functions NormalizeToFQDNRulesConfigFromViper and NormalizeToFQDNRules have been removed without any remaining references or dependencies in the codebase. Their removal does not affect other parts of the project.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of removed normalization functions in the codebase.

# Test: Search for references to the removed functions. Expect: No references found.
rg --type go 'NormalizeToFQDNRulesConfigFromViper|NormalizeToFQDNRules'

Length of output: 73

---

Script:

#!/bin/bash
# Description: Verify the removal of the normalization functions and check for any indirect references.

# Check for any comments or documentation that might reference the removed functions.
rg --type go 'NormalizeToFQDNRulesConfigFromViper|NormalizeToFQDNRules' -g '*.md' -g '*.txt' -g '*.go' -g '*.yaml' -g '*.yml'

# Verify the removal of the functions by searching for their definitions in the codebase.
ast-grep --lang go --pattern 'func NormalizeToFQDNRulesConfigFromViper($_) { $$$ }'
ast-grep --lang go --pattern 'func NormalizeToFQDNRules($_) { $$$ }'

Length of output: 284

hscontrol/handlers.go (2)

181-186: Ensure URL safety in AuthURL.

The AuthURL method constructs a URL using fmt.Sprintf. Ensure that mKey.String() is URL-safe or consider using a URL encoding function.

---

Line range hint 193-216: Review RegisterHandler for security vulnerabilities.

The RegisterHandler method processes user input from the URL. Ensure that all inputs are properly sanitised to prevent XSS or injection attacks.

integration/auth_oidc_test.go (1)

Line range hint 1-1: Verify impact of removed HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN.

The removal of HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN from tests indicates a change in logic. Ensure that the new logic is covered by tests and that the removal does not affect test validity.

hscontrol/types/node_test.go (1)

167-172: Ensure comprehensive FQDN validation.

The test case for overly long usernames is a good addition. Ensure that all edge cases for FQDN validation are covered, such as invalid characters or empty domains.

hscontrol/mapper/mapper_test.go (2)

32-34: LGTM! Improved user profile initialization.

The addition of the Model field in the User struct enhances the robustness of the user profile representation in tests. This aligns with GORM's conventions.

---

79-79: LGTM! Simplified DNS configuration.

The use of an empty Routes map in the DNS configuration response simplifies the test setup and aligns with a more flexible DNS handling approach.

hscontrol/types/node.go (1)

Line range hint 396-411: LGTM! Simplified FQDN generation.

The GetFQDN method now constructs the FQDN using only GivenName and baseDomain, reducing complexity. Ensure that this change aligns with all intended use cases.

hscontrol/oidc.go (9)

50-59: LGTM! Enhanced modularity with AuthProviderOIDC.

The introduction of the AuthProviderOIDC struct encapsulates OIDC functionality, improving modularity and clarity.

---

62-100: LGTM! Centralised OIDC setup.

The NewAuthProviderOIDC constructor effectively initializes the AuthProviderOIDC instance, centralising OIDC setup and enhancing modularity.

---

102-106: LGTM! Correct construction of authentication URL.

The AuthURL method constructs the URL using the server URL and machine key, ensuring correct URL formation.

---

109-114: LGTM! Correct token expiration logic.

The determineTokenExpiration method correctly handles expiration based on configuration, ensuring appropriate token validity.

---

Line range hint 120-167: LGTM! Robust OIDC registration handling.

The RegisterHandler method effectively manages state and redirects for OIDC registration, ensuring robust error handling.

---

Line range hint 190-269: LGTM! Effective OIDC callback handling.

The OIDCCallback method processes the callback, validates parameters, and manages node registration with comprehensive error handling.

Tools

golangci-lint

234-234: Function validateNodeForOIDCCallback should pass the context parameter

(contextcheck)

---

285-296: LGTM! Correct ID token retrieval.

The getIDTokenForOIDCCallback method exchanges the code for a token and extracts the ID token, ensuring proper error handling.

---

302-311: LGTM! Correct ID token verification.

The verifyIDTokenForOIDCCallback method uses the OIDC provider's verifier to ensure the ID token's validity and integrity.

---

434-462: LGTM! Effective user management.

The createOrUpdateUserFromClaim method handles user creation and updates based on OIDC claims, with proper error handling and legacy support.

hscontrol/mapper/mapper.go (2)

97-100: LGTM! Improved user identification.

The generateUserProfiles function now uses user IDs instead of names for identification, enhancing robustness and reducing the risk of conflicts.

---

Line range hint 116-122: LGTM! Simplified DNS configuration logic.

The generateDNSConfig function has been simplified by removing MagicDNS-related logic, reducing complexity. Ensure that this change aligns with all DNS configuration requirements.

hscontrol/db/db.go (2)

406-417: Review new migration entry for types.User.

The migration entry for types.User seems straightforward. Ensure that this migration aligns with the current schema and does not introduce any conflicts.

---

261-261: Verify removal of normalization logic.

The removal of the normalization step for GivenName may affect how node names are stored. Ensure that this change does not introduce any inconsistencies or issues in other parts of the system that rely on normalized names.

hscontrol/db/routes.go (1)

647-647: Ensure consistency with Username() usage.

The change from node.User.Name to node.User.Username() may affect user identity validation. Verify that Username() is consistently used across the codebase to prevent any mismatches or inconsistencies.

Verification successful

---

Consistent Usage of Username() Verified

The method Username() is consistently used across the codebase for accessing user identities internally. The references to claims.Username and u.Name are part of handling external user data, such as from an OIDC provider, and do not conflict with the internal usage of Username(). Thus, the change to node.User.Username() is consistent with the established pattern.

• hscontrol/oidc.go: Handles external claims.
• hscontrol/types/users.go: Maps external claims to internal structures.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify consistent usage of `Username()` for user identity validation.

# Test: Search for all usages of `Username()` to ensure consistency.
rg --type go 'Username()'

Length of output: 1644

---

Script:

#!/bin/bash
# Check if `claims.Username` and `u.Name` are related to the same user identity context as `Username()`.
rg --type go 'claims.Username' -A 3
rg --type go 'u.Name =' -A 3

Length of output: 843

hscontrol/grpcv1.go (3)

43-43: Improved clarity with GetUserByName.

The change to GetUserByName improves code clarity by explicitly indicating that the user is fetched by name. This enhances readability and aligns with best practices.

---

72-72: Improved clarity with GetUserByName.

This change enhances the clarity of the RenameUser function by making it explicit that the user is retrieved by their new name.

---

754-754: Improved clarity with GetUserByName.

The use of GetUserByName in DebugCreateNode clarifies the intention of fetching a user by name, improving code maintainability.

hscontrol/auth.go (2)

476-476: Streamlined AuthURL handling.

The refactoring of AuthURL handling to use h.authProvider.AuthURL(machineKey) simplifies the logic and improves maintainability.

---

701-701: Streamlined AuthURL handling.

This change enhances the handling of authentication URLs by centralising the logic in the AuthProvider interface, promoting modularity.

hscontrol/db/node.go (3)

620-639: Refactoring approved.

The refactoring of generateGivenName improves readability and error handling by directly checking the length of suppliedName and using a specific error for long hostnames.

---

340-340: LGTM! Verify GetUserByName implementation.

The change to use GetUserByName improves clarity. Ensure that GetUserByName is correctly implemented and handles errors appropriately.

Verification successful

---

Verification Successful: GetUserByName Implementation is Correct

The function GetUserByName is correctly implemented. It uses GORM to retrieve a user by name and appropriately handles the case where the user is not found by returning a custom error ErrUserNotFound.

• Location: hscontrol/db/users.go, lines 124-134

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation of `GetUserByName`.

# Test: Search for the function definition to review its implementation.
ast-grep --lang go --pattern 'func GetUserByName($_, $_) ($_, error) { $$$ }'

Length of output: 617

---

393-393: LGTM! Verify Username() implementation.

The change to use node.User.Username() instead of node.User.Name suggests improved encapsulation. Ensure that Username() is correctly implemented and encapsulates necessary logic.

Also applies to: 409-409

hscontrol/policy/acls.go (3)

740-740: Simplification approved.

The removal of group name normalization simplifies the logic and reduces potential errors. Ensure this change does not affect functionality elsewhere.

---

929-929: LGTM! Verify Username() implementation.

The change to use node.User.Username() standardizes user identifier access. Ensure that Username() is correctly implemented.

---

953-953: Standardization approved.

The use of node.User.Username() instead of node.User.Name aligns with the standardization of user identifier access and likely improves data integrity.

hscontrol/app.go (3)

98-99: Modularity improvement approved.

The introduction of authProvider encapsulates authentication logic within a single interface, enhancing modularity and flexibility.

---

444-448: Router setup transition approved.

The use of authProvider.RegisterHandler and conditional OIDC callback setup reflects the transition to the new authentication mechanism, enhancing flexibility.

---

154-178: Enhanced authentication setup approved.

The initialization of authProvider with a fallback mechanism improves error handling and flexibility. Ensure that AuthProvider implementations are correctly integrated.

Tools

golangci-lint

157-157: Magic number: 30, in detected

(gomnd)

hscontrol/types/config.go (4)

74-74: Configuration simplification approved.

The removal of DNSUserNameInMagicDNS simplifies the configuration model, reducing complexity.

---

315-330: Improved error handling approved.

The inclusion of fatal error checks for deprecated keys and improved error messaging enhances error handling and ensures users update their configurations.

---

767-772: Configuration validation improvements approved.

The removal of references to deprecated fields and improved error messaging streamline the configuration validation process and improve clarity.

Also applies to: 806-810

---

918-925: Deprecation handling approved.

The updates to the deprecator struct and its methods ensure proper handling and logging of deprecated keys, guiding users to update their configurations.

Also applies to: 930-937, 945-952, 961-968, 975-980

hscontrol/policy/acls_test.go (2)

651-651: LGTM! Updated expected output.

The expected output for the "Expand emails in group" test case has been updated to reflect full email addresses. This aligns with the intended functionality.

---

651-651: Verify the removal of the email domain stripping test case.

The test case for stripping domains from email addresses has been removed. Ensure that this functionality is either no longer required or covered elsewhere.

Verification successful

---

Removal of email domain stripping test case is justified.

The functionality for stripping email domains appears to have been deprecated, as indicated by the references in hscontrol/types/config.go. Therefore, the removal of the related test case is consistent with the current codebase. No further action is necessary.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if the functionality for stripping email domains is covered elsewhere in the codebase.

# Test: Search for any function or test related to email domain stripping. Expect: At least one occurrence.
rg --type go 'stripEmailDomain' -A 5

Length of output: 36

---

Script:

#!/bin/bash
# Description: Broaden the search to find any functionality related to email processing or domain handling.

# Test: Search for keywords related to email processing or domain handling.
rg --type go 'email' -A 5

Length of output: 4339

Files that changed from the base of the PR and between df9a74d and ccbab98.

• hscontrol/types/users.go (5 hunks)

hscontrol/types/users.go (5)

21-43: Enhancements to User struct approved.

The additions of DisplayName, ProviderIdentifier, Provider, and ProfilePicURL enhance user data management, particularly for OIDC authentication.

---

52-54: Username method implementation approved.

The method effectively prioritises user identifiers and uses strconv.FormatUint for performance.

---

58-59: DisplayNameOrUsername method implementation approved.

The method provides a sensible fallback mechanism for user display names, enhancing user experience.

---

63-64: profilePicURL method implementation approved.

The method encapsulates access to the profile picture URL, promoting encapsulation and reusability.

---

110-132: OIDCClaims struct and FromClaim method implementation approved.

The struct and method ensure user data remains in sync with the authentication provider, enhancing functionality and flexibility.

Files that changed from the base of the PR and between ccbab98 and e78aaa8.

• flake.nix (1 hunks)
• hscontrol/app.go (5 hunks)
• hscontrol/auth.go (11 hunks)
• hscontrol/db/db.go (3 hunks)
• hscontrol/db/node.go (4 hunks)
• hscontrol/db/preauth_keys.go (3 hunks)
• hscontrol/db/routes.go (1 hunks)
• hscontrol/db/users.go (6 hunks)
• hscontrol/db/users_test.go (2 hunks)
• hscontrol/grpcv1.go (3 hunks)
• hscontrol/handlers.go (3 hunks)
• hscontrol/mapper/mapper.go (3 hunks)
• hscontrol/mapper/mapper_test.go (4 hunks)
• hscontrol/mapper/tail.go (1 hunks)
• hscontrol/oidc.go (12 hunks)
• hscontrol/policy/acls.go (3 hunks)
• hscontrol/policy/acls_test.go (3 hunks)
• hscontrol/suite_test.go (1 hunks)
• hscontrol/types/config.go (13 hunks)
• hscontrol/types/config_test.go (4 hunks)
• hscontrol/types/node.go (2 hunks)
• hscontrol/types/node_test.go (4 hunks)
• hscontrol/types/users.go (5 hunks)
• hscontrol/util/dns.go (2 hunks)
• hscontrol/util/dns_test.go (1 hunks)
• integration/auth_oidc_test.go (3 hunks)

• flake.nix
• hscontrol/types/config_test.go
• hscontrol/util/dns_test.go

hscontrol/suite_test.go (1)

49-49: Verify if the change aligns with the intended behaviour and has been thoroughly tested.

Please ensure that this change:

1. Aligns with the intended behaviour of the ResetDB function and the overall test suite.
2. Has been thoroughly tested to ensure it doesn't introduce any regressions in the authentication flow and user management.

To verify the impact of this change, please run the following script:

Please provide the output of running this script and confirm if:

1. There are no other occurrences of StripEmaildomain being set to false.
2. The relevant tests exist, have been updated if necessary, and are passing.

Verification successful

---

Change aligns with intended behaviour but verify test coverage manually.

The change to the OIDC configuration in hscontrol/suite_test.go aligns with the intended behaviour, as there are no conflicting configurations found in the codebase. Tests related to OIDC configuration exist, suggesting that the functionality is being tested. However, it is recommended to manually verify that these tests cover the specific change to ensure comprehensive test coverage.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the OIDC configuration change.

# Test 1: Search for usage of the `StripEmaildomain` property. 
# Expect: No occurrences of `StripEmaildomain` being set to `false`.
rg --type go -A 5 $'StripEmaildomain: false'

# Test 2: Search for tests related to the OIDC configuration. 
# Expect: Relevant tests exist and have been updated if necessary.
rg --type go -A 5 $'OIDC.*Config'

Length of output: 3740

hscontrol/db/users_test.go (2)

23-23: LGTM!

The function name change from GetUser to GetUserByName improves clarity and matches the list of alterations. The test logic remains the same, ensuring that the user is deleted after calling DestroyUser.

---

76-76: LGTM!

The function name change from GetUser to GetUserByName improves clarity and matches the list of alterations. The test logic remains the same, ensuring that the user is renamed and checking for errors when the user is not found or when the new user name already exists.

Also applies to: 79-79

hscontrol/mapper/tail.go (1)

79-81: LGTM!

The code changes are approved. The modification simplifies the GetFQDN method call by only passing the base domain as an argument, streamlining the function's logic. The error handling remains intact, ensuring any failure in generating the FQDN is still reported appropriately.

hscontrol/types/users.go (8)

21-43: LGTM!

The addition of new fields to the User struct improves the representation of user data. The comments on the Name and Email fields provide clear guidance to use alternative methods for accessing the user's identifier.

---

45-54: LGTM!

The Username method provides a clear and consistent way to retrieve the user's identifier based on the availability of Email, Name, ProviderIdentifier, or the user's ID. This ensures a unified approach across the codebase.

---

56-59: LGTM!

The DisplayNameOrUsername method offers a handy way to retrieve the user's display name, falling back to the username if the display name is not available. This ensures a user-friendly representation of the user's identity.

---

64-64: LGTM!

The modification to the profilePicURL method ensures it returns the actual ProfilePicURL when available, aligning with the purpose of the new field.

---

70-71: LGTM!

The updates to the TailscaleUser and TailscaleLogin methods ensure consistent user identification by utilizing the new Username and DisplayNameOrUsername methods for the LoginName and DisplayName fields. This promotes a unified approach across different contexts.

Also applies to: 84-86

---

96-97: LGTM!

The updates to the TailscaleUserProfile method ensure consistent user identification by utilizing the new Username and DisplayNameOrUsername methods for the LoginName and DisplayName fields, promoting a unified approach.

---

110-121: LGTM!

The addition of the OIDCClaims struct enhances the integration of OIDC within the user management system by providing a structured way to handle and access various OIDC claims. This improves the overall functionality and flexibility of user data handling.

---

123-132: LGTM!

The FromClaim method provides a convenient way to update the User fields based on the provided OIDC claims, excluding the ID. This facilitates the synchronization of user data from OIDC claims to the User struct while preserving the existing user ID.

hscontrol/db/users.go (8)

52-52: LGTM!

The change from GetUser to GetUserByName improves clarity and is consistent with the function renaming.

---

93-93: LGTM!

The change from GetUser to GetUserByName improves clarity and is consistent with the function renaming.

---

101-101: LGTM!

The change from GetUser to GetUserByName improves clarity and is consistent with the function renaming.

---

118-122: LGTM!

The function renaming from GetUser to GetUserByName improves clarity and is consistent with the list of alterations.

---

Line range hint 124-134: LGTM!

The function renaming from GetUser to GetUserByName improves clarity and is consistent with the list of alterations.

---

136-140: LGTM!

The new function GetUserByOIDCIdentifier expands the functionality to include user identification via OIDC, as mentioned in the AI-generated summary. It follows a similar pattern to the existing user retrieval methods, ensuring consistency.

---

142-152: LGTM!

The new function GetUserByOIDCIdentifier expands the functionality to include user identification via OIDC, as mentioned in the AI-generated summary. It follows a similar pattern to the existing user retrieval methods, ensuring consistency.

---

176-176: LGTM!

The change from GetUser to GetUserByName improves clarity and is consistent with the function renaming.

hscontrol/db/preauth_keys.go (1)

Line range hint 109-119: LGTM! 🚀

The updates to the ListPreAuthKeys function, similar to the CreatePreAuthKey function, improve the specificity of user identification by explicitly using the user's name. The control flow remains intact, and the changes do not introduce any issues.

hscontrol/util/dns.go (3)

Line range hint 1-1: LGTM: Removal of unused Viper import statement.

The removal of the Viper import statement is approved as the associated functions NormalizeToFQDNRulesConfigFromViper and NormalizeToFQDNRules have been removed from the file. This change simplifies the codebase by removing the dependency on the Viper configuration management library.

---

Line range hint 1-1: Verify the impact of removing the NormalizeToFQDNRulesConfigFromViper function.

The removal of the NormalizeToFQDNRulesConfigFromViper function indicates a shift in how user names are processed. Please ensure that:

1. There are no remaining calls to this function in the codebase, as they will result in errors.
2. The removal of this function does not break any existing functionality or impact the overall user experience.

Run the following script to verify the impact of removing the function:

---

Line range hint 1-1: Verify the impact of removing the NormalizeToFQDNRules function.

The removal of the NormalizeToFQDNRules function indicates a shift in how user names are processed. Please ensure that:

1. There are no remaining calls to this function in the codebase, as they will result in errors.
2. The removal of this function does not break any existing functionality or impact the overall user experience.

Run the following script to verify the impact of removing the function:

Verification successful

---

Verification Successful: NormalizeToFQDNRules Function Removed

The NormalizeToFQDNRules function and all its calls have been successfully removed from the codebase. No remaining references to this function were found, indicating that its removal does not impact the current functionality.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing the `NormalizeToFQDNRules` function.

# Test 1: Search for any remaining calls to the removed function. Expect: No results.
rg --type go $'NormalizeToFQDNRules'

# Test 2: Search for the implementation of the removed function. Expect: No results.
ast-grep --lang go --pattern $'func NormalizeToFQDNRules($_, $_) {
  $$$
}'

Length of output: 112

---

Script:

#!/bin/bash
# Re-run the verification to ensure the function and its calls are absent.

# Test 1: Search for any remaining calls to the removed function. Expect: No results.
rg --type go 'NormalizeToFQDNRules'

# Test 2: Search for the implementation of the removed function. Expect: No results.
ast-grep --lang go --pattern 'func NormalizeToFQDNRules($_, $_) {
  $$$
}'

Length of output: 112

hscontrol/handlers.go (3)

171-173: LGTM!

The code changes are approved.

---

181-186: LGTM!

The code changes are approved.

---

Line range hint 193-257: LGTM!

The code changes are approved.

integration/auth_oidc_test.go (2)

Line range hint 42-42: Verify the impact of removing the StripEmaildomain field.

Ensure that removing the StripEmaildomain field from the OIDCConf structure does not affect the expected behaviour of the OIDC authentication tests.

Run the following script to verify the impact:

Verification successful

---

Removal of StripEmaildomain Field Verified

The StripEmaildomain field is not used elsewhere in the codebase, confirming that its removal does not impact the functionality of the OIDC authentication tests or other components.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing the `StripEmaildomain` field.

# Test: Search for usages of the `StripEmaildomain` field. Expect: No occurrences.
rg --type go $'StripEmaildomain'

Length of output: 31

---

Line range hint 51-51: Verify the impact of removing the HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN configuration.

Ensure that removing the HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN configuration from the test cases does not affect the expected behaviour of the OIDC authentication tests.

Run the following script to verify the impact:

Also applies to: 84-84

Verification successful

---

Removal of HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN Configuration Verified

The removal of the HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN configuration does not impact the expected behaviour of the OIDC authentication tests. The tests are functioning with the current OIDC setup, which does not include this configuration.

• The test cases TestOIDCAuthenticationPingAll and TestOIDCExpireNodesBasedOnTokenExpiry do not reference this configuration and are designed to work with the existing OIDC configurations.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing the `HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN` configuration.

# Test: Search for usages of the `HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN` configuration. Expect: No occurrences.
rg --type go $'HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN'

Length of output: 48

---

Script:

#!/bin/bash
# Description: Search for any related configurations or logic that might have replaced `HEADSCALE_OIDC_STRIP_EMAIL_DOMAIN`.

# Search for any configuration or logic related to OIDC email handling.
rg --type go 'OIDC' -A 5

# Search for the test cases `TestOIDCAuthenticationPingAll` and `TestOIDCExpireNodesBasedOnTokenExpiry` to understand their current behaviour.
rg 'func TestOIDCAuthenticationPingAll' -A 20
rg 'func TestOIDCExpireNodesBasedOnTokenExpiry' -A 20

Length of output: 36161

hscontrol/types/node_test.go (4)

167-172: LGTM!

The new test case is correctly testing the behaviour of the GetFQDN function when the username is very long. It expects the function to return an error indicating that the hostname is too long and cannot exceed 255 ASCII characters.

---

189-190: LGTM!

The removal of the cfg parameter from the GetFQDN function call simplifies the test structure by eliminating the configuration setup for each test case. The function now only requires the domain parameter.

---

191-191: LGTM!

The log statement is useful for debugging purposes and can help in understanding the values returned by the GetFQDN function.

---

Line range hint 193-203:

hscontrol/mapper/mapper_test.go (3)

79-79: Simplification of DNS configuration handling.

The following changes simplify the DNS configuration handling:

• The Routes field in the DNSConfig struct is initialized as an empty map instead of containing predefined routes, allowing for more dynamic handling of DNS routes.
• The DNSUserNameInMagicDNS field is removed from the Config struct instantiation, simplifying the configuration parameters.

Also applies to: 129-129

---

Line range hint 141-433:

---

15-15: Significant change: User struct now includes gorm.Model.

The User struct now includes a Model field of type gorm.Model, which is a significant change that enables ORM operations with GORM.

Verify the impact of this change on the codebase by running the following script:

Also applies to: 32-34

hscontrol/types/node.go (1)

Line range hint 396-420: Simplification of the GetFQDN method's interface

The removal of the cfg parameter simplifies the method's interface and reduces coupling with the Config struct, which improves the method's modularity and testability.

However, the removal of the cfg.DNSUserNameInMagicDNS logic changes the behavior of the method, as it no longer considers the user's name when generating the FQDN.

Please verify that this change does not introduce any unintended side effects in the system. You can use the following script to search for any potential issues:

Verification successful

---

Verification of GetFQDN method changes

The removal of the cfg.DNSUserNameInMagicDNS logic from the GetFQDN method does not appear to affect other parts of the codebase, as there are no remaining references to this logic. Additionally, the call to GetFQDN in hscontrol/mapper/tail.go aligns with the new method signature, using cfg.BaseDomain appropriately.

Please ensure that the change in behaviour due to the removal of cfg.DNSUserNameInMagicDNS logic does not introduce any unintended side effects in the system.

• File to review: hscontrol/mapper/tail.go

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for potential issues related to the removal of the `cfg.DNSUserNameInMagicDNS` logic.

# Test 1: Search for any references to `cfg.DNSUserNameInMagicDNS` in the codebase.
# Expect: No references found.
rg --type go $'cfg\.DNSUserNameInMagicDNS'

# Test 2: Search for any calls to `GetFQDN` that still pass a `cfg` parameter.
# Expect: No calls found.
rg --type go $'GetFQDN\(.*cfg.*\)'

Length of output: 151

hscontrol/mapper/mapper.go (2)

97-100: LGTM!

The change from using user names to user IDs as keys in the userMap is an improvement. It enhances the uniqueness of user identification and reduces the risk of collisions that could arise from users having the same name. The update to the loop that populates the userMap for peers maintains consistency with the new keying strategy.

---

125-125: Please verify the impact of removing the DNS configuration logic.

The removal of the substantial block of code that handles the configuration of DNS settings when MagicDNS is enabled suggests a shift in how DNS configurations are managed. It potentially indicates a move towards a more simplified or different approach to DNS handling within the application.

To verify the impact of this change, please run the following script:

Please provide the results of running these verification tests so that we can assess the impact of this change on the overall DNS configuration management.

Verification successful

---

The removal of DNS configuration logic is covered by existing tests.

The TestDNSConfigMapResponse test case includes scenarios for DNS configuration when magicDNS is enabled or disabled, ensuring that the impact of the removed logic is verified through these tests. No further action is required unless additional test coverage is deemed necessary.

• The TestDNSConfigMapResponse test case covers the Proxied field and domain settings.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of removing the DNS configuration logic on the overall system.

# Test 1: Search for occurrences of `dnsConfig.Proxied` in the codebase.
# Expect: No occurrences, indicating that the removed logic is not used elsewhere.
rg --type go $'dnsConfig\.Proxied'

# Test 2: Search for occurrences of DNS route construction based on user names.
# Expect: No occurrences, indicating that the removed logic is not used elsewhere.
rg --type go $'dnsConfig\.Routes\[[^]]+\] = .*\.Name'

# Test 3: Verify if there are any test cases covering the removed DNS configuration logic.
# Expect: No test cases, indicating that the removed logic was not actively tested.
fd _test.go --exec rg $'func Test.*DNS.*Config'

Length of output: 185

---

Script:

#!/bin/bash
# Description: Extract the details of the TestDNSConfigMapResponse test case to assess its relevance to the removed DNS configuration logic.

# Locate the file containing the TestDNSConfigMapResponse test case and display its content.
fd _test.go --exec rg -A 20 $'func TestDNSConfigMapResponse'

Length of output: 491

hscontrol/oidc.go (15)

50-59: LGTM!

The introduction of the AuthProviderOIDC structure is a great way to encapsulate the OIDC-related components and improve the modularity of the code. Well done!

---

62-100: LGTM!

The NewAuthProviderOIDC constructor is a great addition that streamlines the initialization process for the AuthProviderOIDC structure. Moving the OIDC provider and OAuth2 configuration setup into this constructor enhances the clarity and maintainability of the code. Nice work!

---

102-106: LGTM!

The modification of the AuthURL method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. Good job!

---

109-114: LGTM!

The modification of the determineTokenExpiration method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. Well done!

---

Line range hint 119-167: LGTM!

The modification of the RegisterHandler method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. Good job!

---

Line range hint 190-269: LGTM!

The modification of the OIDCCallback method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. The improved error handling, where errors are returned instead of writing responses directly, is a great change that enhances the flexibility and maintainability of the code. Well done!

---

285-298: LGTM!

The modification of the getIDTokenForOIDCCallback method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. The improved error handling, where errors are returned instead of writing responses directly, is a great change that enhances the flexibility and maintainability of the code. Good job!

---

302-312: LGTM!

The modification of the verifyIDTokenForOIDCCallback method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. The improved error handling, where errors are returned instead of writing responses directly, is a great change that enhances the flexibility and maintainability of the code. Well done!

---

319-319: LGTM!

The modification of the validateOIDCAllowedDomains function to accept types.OIDCClaims instead of IDTokenClaims aligns with the overall refactoring and enhances the type safety of the code. Good job!

---

337-337: LGTM!

The modification of the validateOIDCAllowedGroups function to accept types.OIDCClaims instead of IDTokenClaims aligns with the overall refactoring and enhances the type safety of the code. Well done!

---

356-356: LGTM!

The modification of the validateOIDCAllowedUsers function to accept types.OIDCClaims instead of IDTokenClaims aligns with the overall refactoring and enhances the type safety of the code. Good job!

---

Line range hint 371-438: LGTM!

The modification of the validateNodeForOIDCCallback method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. Well done!

---

444-474: LGTM!

The modification of the createOrUpdateUserFromClaim method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. Good job!

---

Line range hint 477-507: LGTM!

The modification of the registerNodeForOIDCCallback method to use the AuthProviderOIDC receiver aligns with the overall refactoring and enhances the modularity of the code. Well done!

---

511-511: LGTM!

The modification of the renderOIDCCallbackTemplate function to accept types.OIDCClaims instead of IDTokenClaims aligns with the overall refactoring and enhances the type safety of the code. Good job!

hscontrol/db/routes.go (1)

647-647: LGTM!

The change from comparing approvedAlias with node.User.Name to node.User.Username() appears to be intentional and aligns with the goal of improving user identification accuracy. The overall logic flow remains intact, but the semantic meaning of the comparison has changed, which could affect the behaviour of the route approval process. However, this change seems reasonable and well-aligned with the codebase.

hscontrol/db/db.go (2)

413-424: LGTM!

The new migration entry enhances the database schema management by automatically creating or updating the User table structure as needed. This is a useful addition that ensures the database schema stays in sync with the application's data model.

---

268-268: Verify the impact of removing the hostname normalization.

The change simplifies the assignment logic by directly assigning the Hostname to GivenName without any normalization. This may impact how hostnames are stored in the database, potentially leading to inconsistencies if the original hostname format is not standardized.

Run the following script to verify the hostname format of existing nodes in the database:

hscontrol/grpcv1.go (3)

44-44: LGTM!

The method name change from GetUser to GetUserByName improves clarity by explicitly indicating that the user is retrieved by name.

---

73-73: LGTM!

The method name change is consistent with the previous update and improves clarity.

---

777-777: LGTM!

The method name change is consistent with the previous updates and improves clarity.

hscontrol/auth.go (5)

21-24: LGTM!

The new AuthProvider interface provides a clean abstraction for authentication providers. The RegisterHandler and AuthURL methods enable flexibility in implementing custom authentication flows.

---

182-182: LGTM!

The removal of the unused machineKey parameter simplifies the handleNodeLogOut function signature.

---

190-190: LGTM!

The removal of the unused machineKey parameter simplifies the handleNodeWithValidRegistration function signature.

---

476-476: LGTM!

Using h.authProvider.AuthURL(machineKey) to generate the authentication URL is a good improvement. It centralizes the URL generation logic within the AuthProvider interface, promoting better encapsulation and simplifying future modifications to the authentication process.

---

701-701: LGTM!

Using h.authProvider.AuthURL(machineKey) to generate the authentication URL in the handleNodeExpiredOrLoggedOut function is a good improvement. It centralizes the URL generation logic within the AuthProvider interface, promoting better encapsulation and simplifying future modifications to the authentication process.

hscontrol/db/node.go (3)

340-340: LGTM!

The change from GetUser to GetUserByName improves code clarity by explicitly indicating that the user is retrieved by name.

---

393-393: LGTM!

The change from node.User.Name to node.User.Username() improves code consistency by using the Username() method instead of directly accessing the Name field.

Also applies to: 409-409

---

620-621: LGTM!

The changes to the generateGivenName function simplify the logic and improve error handling:

• Removing the hostname normalization step and directly checking the length of the supplied name against util.LabelHostnameLength simplifies the function.
• Returning an error if the supplied name exceeds the length limit ensures that invalid hostnames are not generated.
• Operating directly on the suppliedName variable instead of the previously normalized hostname reduces complexity.

Also applies to: 627-628, 636-636, 639-639

hscontrol/policy/acls.go (3)

929-929: LGTM!

The change is consistent with the modification in the filterNodesByUser function and suggests a shift towards a more robust or standardized method for retrieving usernames.

---

953-953: LGTM!

The change is consistent with the modification in the TagsOfNode function and suggests a shift towards a more robust or standardized method for retrieving usernames.

---

740-740: Verify the impact of removing the group name normalization.

The change simplifies the function by eliminating error handling related to group normalization. However, this may impact how groups are processed if they are not in the expected format.

Run the following script to verify the usage of the expandUsersFromGroup function:

Consider adding a comment to document the expected group format to avoid potential issues.

Verification successful

---

Verify test coverage for group name formatting in expandUsersFromGroup.

The removal of group name normalization could affect the function's reliability if group names are not in the expected format. However, error handling and tests suggest that this is managed. Ensure that tests cover scenarios with improperly formatted group names to maintain robustness.

• Check hscontrol/policy/acls_test.go for test cases involving group name formatting.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the `expandUsersFromGroup` function.

# Test: Search for the function usage. Expect: No issues with the group format.
rg --type go -A 5 $'expandUsersFromGroup'

Length of output: 1909

hscontrol/app.go (4)

98-98: LGTM!

The changes to the Headscale struct are approved. The removal of the oidcProvider and oauth2Config fields and the addition of the authProvider field of type AuthProvider centralise the authentication logic.

---

154-178: LGTM!

The changes to the NewHeadscale function are approved. The new authentication logic enhances the robustness of the authentication process by:

• Adding a timeout context for the OIDC provider setup.
• Attempting to initialize the oidcProvider and assigning it to authProvider if the OIDC issuer is specified.
• Falling back to CLI-based authentication if the OIDC initialization fails, maintaining backward compatibility.

---

444-448: LGTM!

The changes to the createRouter function are approved. The routing logic has been updated to:

• Directly call h.authProvider.RegisterHandler for the registration endpoint.
• Conditionally route the OIDC callback based on the type of authProvider.

These changes streamline the routing process and encapsulate the authentication logic within the AuthProvider interface, promoting better separation of concerns.

---

Line range hint 1-1000: Skipped reviewing the remaining functions.

The remaining functions in the file have not been modified, so they do not require a review.

hscontrol/types/config.go (13)

74-74: LGTM!

The code change simplifies the configuration by removing the DNSUserNameInMagicDNS field and adding the DNSConfig field to directly use the Tailscale DNS configuration type.

---

92-96: LGTM!

The code change simplifies the DNS configuration by removing the UserNameInMagicDNS field and adding the relevant DNS configuration fields directly to the DNSConfig struct.

---

314-316: LGTM!

The code change removes the deprecated configuration keys and uses the fatal method to handle the removal, ensuring that any attempts to use these keys will result in a fatal error.

---

320-328: LGTM!

The code change adds an additional check to ensure that the removed configuration keys are not present in the configuration file. If any of the removed keys are found, a fatal error is logged, prompting the user to update their configuration file.

---

770-775: LGTM!

The code change improves the error handling for the prefixes.allocation configuration key. It checks if the configured value is a valid allocation strategy and returns a descriptive error message if an invalid value is provided, including the allowed options.

---

809-813: LGTM!

The code change adds a validation check to ensure that the server_url does not contain the base_domain value. This is important because if the server_url contains the base_domain, it can cause the headscale server and embedded DERP to become unreachable from the Tailscale node. The error message provides a clear explanation of the issue.

---

843-843: LGTM!

The code change sets the DNSConfig field of the Config struct by calling the DNSToTailcfgDNS function with the dnsConfig variable. This ensures that the DNS configuration is properly converted to the Tailscale DNS configuration format.

---

855-862: LGTM!

The code change sets the OIDC configuration fields of the OIDCConfig struct using the values from the corresponding configuration keys. This ensures that the OIDC configuration is properly populated based on the provided configuration values.

---

897-901: LGTM!

The code change sets the tuning configuration fields of the Tuning struct using the values from the corresponding configuration keys. This ensures that the tuning parameters are properly populated based on the provided configuration values.

---

921-928: LGTM!

The code change adds a warning message to the warns set of the deprecator if the old configuration key is set. The warning message informs the user about the deprecated key, the new key to use instead, and that the old key will be removed in the future. This helps in communicating the deprecation of configuration keys to the user.

---

933-940: LGTM!

The code change adds a fatal message to the fatals set of the deprecator if the old configuration key is set. The fatal message informs the user that the old key has been removed and directs them to the changelog for more details. This helps in communicating the removal of configuration keys to the user and ensures that the application fails if a removed key is still being used.

---

948-955: LGTM!

The code change adds a fatal message to the fatals set of the deprecator if the old configuration key is set and the new key is not set. The fatal message informs the user about the deprecated key, the new key to use instead, and that the old key has been removed. This helps in communicating the removal of configuration keys to the user and ensures that the application fails if a removed key is still being used without the new key being set.

---

964-971: **LGTM!

hscontrol/policy/acls_test.go (1)

651-651: Expected output for email expansion test corrected.

The expected result for the "Expand emails in group" test case has been updated from []string{"joe.bar.gmail.com", "john.doe.yahoo.fr"} to []string{"[email protected]", "[email protected]"}.

This correction ensures that the test accurately reflects the intended behaviour of the email expansion feature, which should return the full email addresses rather than just the usernames.

Files that changed from the base of the PR and between e78aaa8 and d0adeb9.

• hscontrol/types/users.go (5 hunks)

• hscontrol/types/users.go

Files that changed from the base of the PR and between d0adeb9 and 2a3e21f.

• CHANGELOG.md (1 hunks)

CHANGELOG.md (3)

7-8: Clarify the impact of removing dns.use_username_in_magic_dns.

The removal of the dns.use_username_in_magic_dns configuration option is noted as a breaking change. It would be beneficial to provide more context on how this change affects existing configurations and whether any migration steps are necessary for users relying on this feature.

---

10-11: Detail the transition to using the sub claim for user identification.

The shift from using the username to the sub claim in the ID token for OIDC is a significant change. It's important to ensure that all documentation and user guides reflect this new method of user identification to prevent confusion and to aid in a smooth transition for users.

---

12-14: Expand on the implications of new user fields.

The addition of new fields such as username, display name, profile picture URL, and email to the user entity is a notable enhancement. Clarify how these fields are utilised within the system, particularly how they interact with existing data structures and whether they require any database migrations or additional configuration.

Files that changed from the base of the PR and between 2a3e21f and 7c6f4e6.

• CHANGELOG.md (1 hunks)

Files that changed from the base of the PR and between 7c6f4e6 and e66d149.

• CHANGELOG.md (1 hunks)
• flake.nix (1 hunks)
• hscontrol/app.go (3 hunks)
• hscontrol/auth.go (5 hunks)
• hscontrol/db/db.go (2 hunks)
• hscontrol/db/node.go (4 hunks)
• hscontrol/db/preauth_keys.go (3 hunks)
• hscontrol/db/routes.go (1 hunks)
• hscontrol/db/users.go (6 hunks)
• hscontrol/db/users_test.go (2 hunks)
• hscontrol/grpcv1.go (3 hunks)
• hscontrol/handlers.go (3 hunks)
• hscontrol/mapper/mapper.go (1 hunks)
• hscontrol/mapper/mapper_test.go (4 hunks)
• hscontrol/mapper/tail.go (1 hunks)
• hscontrol/oidc.go (11 hunks)
• hscontrol/policy/acls.go (3 hunks)
• hscontrol/policy/acls_test.go (2 hunks)
• hscontrol/suite_test.go (1 hunks)
• hscontrol/types/config.go (10 hunks)
• hscontrol/types/config_test.go (4 hunks)
• hscontrol/types/node.go (1 hunks)
• hscontrol/types/node_test.go (2 hunks)
• hscontrol/types/users.go (5 hunks)
• hscontrol/util/dns.go (0 hunks)
• hscontrol/util/dns_test.go (0 hunks)
• integration/auth_oidc_test.go (0 hunks)

• hscontrol/util/dns.go
• hscontrol/util/dns_test.go
• integration/auth_oidc_test.go

• hscontrol/grpcv1.go
• hscontrol/mapper/tail.go
• hscontrol/types/config_test.go

hscontrol/suite_test.go (1)

49-49: Simplification of OIDC configuration looks good, but verify the impact on email domain handling.

The change simplifies the initialisation of the OIDC field by using an empty OIDCConfig struct, which improves readability. However, it's important to ensure that the removal of the explicit StripEmaildomain property setting aligns with the expected behaviour.

Run the following script to verify the impact on email domain handling:

Verification successful

---

Approval: OIDC configuration change aligns with broader updates to email domain handling.

The removal of the explicit StripEmaildomain property is part of a larger change in how email domains are processed. The OIDCConfig struct now includes AllowedDomains, which appears to be the new approach for handling email domains. Recent commits also indicate ongoing updates to user-related functionality, including email normalisation.

Please ensure that:

• The new AllowedDomains approach meets all requirements previously handled by StripEmaildomain.
• Any code relying on the old StripEmaildomain behaviour has been updated accordingly.
• Documentation is updated to reflect these changes in email domain handling.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the OIDC configuration change on email domain handling.

# Test 1: Search for the definition of the `OIDCConfig` struct. 
# Expect: To find the default value of the `StripEmaildomain` property.
rg --type go 'type OIDCConfig struct'

# Test 2: Search for usage of the `StripEmaildomain` property. 
# Expect: To find instances where the property is used to handle email domains.
rg --type go 'StripEmaildomain'

Length of output: 122

---

Script:

#!/bin/bash
# Description: Further investigate the OIDC configuration and email handling

# View the contents of the OIDCConfig struct
echo "Contents of OIDCConfig struct:"
sed -n '/type OIDCConfig struct/,/}/p' hscontrol/types/config.go

# Search for email-related properties or methods in OIDC configuration
echo -e "\nEmail-related properties or methods in OIDC configuration:"
rg --type go -i 'email|domain' hscontrol/types/config.go

# Look for recent changes related to OIDC configuration or email handling
echo -e "\nRecent changes related to OIDC or email handling:"
git log -n 5 --grep="OIDC\|email\|domain" --pretty=format:"%h - %s" -- hscontrol/

Length of output: 2823

hscontrol/db/users_test.go (3)

23-23: LGTM!

The change in function name from GetUser to GetUserByName improves clarity and maintainability by explicitly indicating that the user is retrieved by their name.

---

76-76: Looks good!

The updated function name GetUserByName accurately reflects the functionality and maintains the test's integrity in asserting that the user is not found under the old name after renaming.

---

79-79: Looks good to me!

The updated function name GetUserByName accurately reflects the functionality and maintains the test's integrity in asserting that the user is found under the new name after renaming.

hscontrol/types/users.go (7)

51-53: Looks good!

The Username() method provides a consistent and efficient way to retrieve the username based on the available user data. The order of preference ensures that the most relevant identifier is used.

---

55-58: Looks good!

The DisplayNameOrUsername() method enhances the user experience by providing a user-friendly display name when available, while falling back to the username if necessary. This ensures a consistent and informative representation of the user throughout the application.

---

109-120: Looks good!

The introduction of the OIDCClaims struct is a valuable addition to the codebase, as it facilitates the integration of OpenID Connect authentication. By mapping the OIDC claims to the User struct, it enables seamless user management and authentication using OIDC providers.

---

122-131: Looks good!

The FromClaim() method is a crucial component in the integration of OpenID Connect authentication. By mapping the OIDC claims to the corresponding fields in the User struct, it ensures that the user data is accurately updated based on the information provided by the OIDC provider. The decision to preserve the existing user ID is a good approach to maintain consistency.

---

69-70: Looks good!

The updates to the TailscaleUser() method ensure that the most relevant and user-friendly values are used for the LoginName and DisplayName fields in the tailcfg.User struct. By leveraging the newly introduced Username() and DisplayNameOrUsername() methods, it provides a consistent and informative representation of the user in the Tailscale context.

---

83-85: Looks good!

The updates to the TailscaleLogin() method ensure that the most relevant and user-friendly values are used for the LoginName and DisplayName fields in the tailcfg.Login struct. By leveraging the newly introduced Username() and DisplayNameOrUsername() methods, it provides a consistent and informative representation of the user's login information in the Tailscale context.

---

95-96: Looks good!

The updates to the TailscaleUserProfile() method ensure that the most relevant and user-friendly values are used for the LoginName and DisplayName fields in the tailcfg.UserProfile struct. By leveraging the newly introduced Username() and DisplayNameOrUsername() methods, it provides a consistent and informative representation of the user's profile information in the Tailscale context.

hscontrol/db/users.go (4)

52-52: Brilliant! The function name change enhances code clarity.

Renaming GetUser to GetUserByName clearly conveys that the user is retrieved based on their name. This improves code readability and maintainability.

---

93-93: Excellent! The consistent function name change enhances code clarity.

Renaming GetUser to GetUserByName within the RenameUser function maintains consistency and improves code readability. This change aligns with the previous function name update.

Also applies to: 101-101

---

118-122: Fantastic! The consistent function name change enhances code clarity.

Renaming GetUser to GetUserByName in both the HSDatabase method and the standalone function maintains consistency and improves code readability. This change aligns with the previous function name updates.

Also applies to: 124-134

---

136-152: Superb! The new functions extend the functionality to support OIDC.

The addition of GetUserByOIDCIdentifier functions as both an HSDatabase method and a standalone function enhances the capability to retrieve users based on their OIDC identifier. This extension aligns with the existing GetUserByName functions and maintains a consistent implementation approach.

hscontrol/db/preauth_keys.go (3)

Line range hint 25-40: Acknowledge the TODO comment for future improvement.

The TODO comment correctly identifies an opportunity to enhance the function by using a user ID instead of a name to uniquely identify users. This change would make the function more robust and less prone to issues arising from non-unique user names.

---

47-47: The function renaming improves code clarity.

Renaming GetUser to GetUserByName enhances code readability by explicitly indicating that the user is being retrieved by their name. This change aligns with the TODO comment about using a user ID in the future and is a step towards more robust user identification.

---

109-109: The function renaming improves code consistency and clarity.

Renaming GetUser to GetUserByName enhances code readability by explicitly indicating that the user is being retrieved by their name. This change is consistent with the similar change made in the CreatePreAuthKey function and improves overall code consistency.

flake.nix (1)

35-35: Spot on! The vendorHash update is necessary and appreciated.

Updating the vendorHash after modifying go.mod or go.sum is crucial for maintaining the integrity of the build process. This change ensures that the correct dependencies are being used, preventing potential build failures or unexpected behaviour. Brilliant work following the best practice mentioned in the comment!

hscontrol/handlers.go (3)

181-186: LGTM!

The AuthURL method correctly constructs the URL by appending the machine key to the server URL. The use of strings.TrimSuffix to remove any trailing slash is a nice touch.

---

Line range hint 193-252: LGTM!

The RegisterHandler method looks good:

• It correctly extracts and validates the machine key from the request URL.
• It has appropriate error handling in place if the machine key is invalid.
• It renders the HTML template correctly with the machine key.
• It sets the response headers and status codes appropriately.

---

208-208: LGTM!

The change in the error message from "nodekey" to "machinekey" improves clarity and consistency in the codebase.

hscontrol/types/node_test.go (5)

Line range hint 14-90:
The Test_NodeCanAccess function has not been modified in this diff. Skipping review.

---

167-172: Excellent addition of a new test case!

The new test case for handling the scenario where the username exceeds the maximum allowable length for a hostname is a great addition to improve the test coverage. Well done!

---

Line range hint 206-312:
The TestPeerChangeFromMapRequest function has not been modified in this diff. Skipping review.

---

Line range hint 314-400:
The TestApplyPeerChange function has not been modified in this diff. Skipping review.

---

Line range hint 92-204: Verify the impact of the removed test cases.

Several test cases related to the inclusion of the username in the FQDN generation have been removed. This suggests a change in the expected behaviour of the GetFQDN method.

Please ensure that the removal of these test cases aligns with the intended behaviour of the GetFQDN method and does not introduce any regressions in the codebase.

Verification successful

---

Removal of username-related test cases for GetFQDN is consistent with implementation

The verification process confirms that the removal of test cases related to including the username in FQDN generation aligns with the current implementation of the GetFQDN method. The method now only uses the GivenName field, and this behaviour is consistently reflected across the codebase. The remaining test cases in hscontrol/types/node_test.go provide adequate coverage for the current functionality, including successful FQDN generation and error handling scenarios.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the impact of the removed test cases on the `GetFQDN` method and the codebase.

# Test: Search for usages of the `GetFQDN` method in the codebase.
rg --type go -A 5 $'GetFQDN'

# Test: Search for test cases that rely on the username being included in the FQDN.
rg --type go -A 5 $'GetFQDN.*User'

Length of output: 1584

hscontrol/mapper/mapper_test.go (3)

32-34: LGTM!

The addition of the Model field from GORM in the User struct is a positive change that enhances the representation of user profiles and potentially improves integration with database operations. The test logic remains unchanged, indicating that the core functionality of generating user profiles is unaffected by this struct change.

---

Line range hint 141-432:

---

Line range hint 79-129: Verify the impact of the DNS configuration changes.

The test case has been updated with the following changes:

1. The Routes field of the tailcfg.DNSConfig struct is now an empty map.
2. The DNSUserNameInMagicDNS field has been removed from the Config struct instantiation.

Please ensure that these changes align with the intended behaviour and do not introduce any unintended side effects in the DNS configuration process.

To verify the impact of these changes, consider running the following script:

hscontrol/mapper/mapper.go (2)

Line range hint 97-110: Excellent change to use user IDs for constructing the userMap.

Using user IDs instead of user names enhances the uniqueness and reliability of user identification, as IDs are less prone to duplication compared to names. The logic for populating the userMap remains largely the same, iterating through peers to add their corresponding users.

This change should not have any negative impact on the functionality and is a positive improvement.

---

Line range hint 112-128: Verify the impact of the removed MagicDNS code block.

The removal of the substantial code block related to MagicDNS functionality indicates a shift in how DNS configurations are generated. It possibly simplifies the logic or alters the intended functionality regarding DNS routing for users.

While the remaining code suggests that the core functionality of DNS configuration remains intact, it's unclear from the provided context how this change impacts the overall DNS functionality.

Please ensure that this removal does not break any existing functionality and that the DNS configuration generation still aligns with the intended system behaviour. Consider running the following script to analyse the impact:

hscontrol/oidc.go (12)

50-59: The AuthProviderOIDC struct is well-structured and modular.

The struct encapsulates various components required for OIDC authentication, such as the server URL, OIDC configuration, database access, registration cache, notifier, IP allocator, OIDC provider, and OAuth2 configuration. Each field has a specific responsibility, making the code more maintainable and easier to understand.

---

62-100: The NewAuthProviderOIDC function is a well-designed constructor.

The function takes in all the necessary parameters to initialize the AuthProviderOIDC struct. It creates a new OIDC provider using the provided issuer configuration and initializes the OAuth2 configuration with the appropriate settings. The function returns a new AuthProviderOIDC instance with all the fields properly initialized, ensuring a clean and consistent setup.

---

102-106: The AuthURL method generates the authentication URL correctly.

The method takes a machine key as input and constructs the authentication URL by appending the machine key to the server URL. It ensures that the server URL is properly formatted by trimming any trailing slash, preventing potential issues with URL construction.

---

109-114: The determineTokenExpiration method correctly determines the token expiration time.

The method takes the ID token expiration time as input and determines the token expiration time based on the configuration. If the UseExpiryFromToken flag is set, it returns the ID token expiration time. Otherwise, it calculates the expiration time by adding the configured expiry duration to the current time. This allows flexibility in determining the token expiration based on the specific requirements.

---

Line range hint 119-170: The RegisterHandler method handles the OIDC registration process effectively.

The method extracts the machine key from the request URL and validates it to ensure it's a valid key. It generates a random state string and stores the machine key in the registration cache using the state as the key. This allows the callback handler to retrieve the machine key later. The method constructs the authorization URL with the necessary parameters, including any extra parameters provided in the configuration, and redirects the user to the OIDC provider for authentication. The code is well-structured and follows the OIDC registration flow correctly.

---

Line range hint 190-270: The OIDCCallback method handles the OIDC callback process comprehensively.

The method follows the OIDC callback flow step by step. It validates the callback parameters, exchanges the authorization code for an ID token, and verifies the ID token. It extracts the claims from the ID token and validates them against the allowed domains, groups, and users specified in the configuration. This ensures that only authorized users are allowed to register nodes.

The method then validates the node associated with the state parameter and registers it if it's a new node. It creates or updates the user based on the claims and registers the node, associating it with the user.

Finally, it renders a callback template with the user information to provide feedback to the user.

The code is well-structured, handles errors appropriately, and covers all the necessary steps in the OIDC callback process.

---

285-299: The getIDTokenForOIDCCallback method retrieves the ID token correctly.

The method takes the authorization code as input and exchanges it for an OAuth2 token using the OIDC provider. It then extracts the ID token from the OAuth2 token. If the ID token is missing, it returns an appropriate error. The code is straightforward and follows the expected flow for retrieving the ID token.

---

302-312: The verifyIDTokenForOIDCCallback method verifies the ID token securely.

The method takes the raw ID token as input and creates a verifier using the OIDC provider and the client ID from the configuration. It then verifies the ID token using the verifier. If the verification fails, it returns an appropriate error. This ensures that only valid and authentic ID tokens are accepted, enhancing the security of the authentication process.

---

Line range hint 371-441: The validateNodeForOIDCCallback method validates the node effectively.

The method retrieves the machine key from the registration cache using the state parameter. If the machine key is not found, it returns an appropriate error. It then retrieves the node information from the database using the machine key.

If the node already exists, it updates the node's expiry and notifies the node and its peers about the expiry update. This ensures that the node's expiry is properly refreshed and synchronized across the network.

If the node doesn't exist, it returns the machine key and a flag indicating that it's a new node, allowing the caller to proceed with the registration process.

The code handles both existing and new nodes correctly and takes care of updating the expiry and sending notifications when necessary.

---

444-474: The createOrUpdateUserFromClaim method handles user creation and updates effectively.

The method retrieves the user from the database using the OIDC identifier from the claims. If the user is not found, it attempts to retrieve the user by username for legacy compatibility. This ensures a smooth transition for existing users.

If the user still doesn't exist, it creates a new empty user. It then updates the user's fields based on the claims, ensuring that the user's information is up to date.

Finally, it saves the user to the database, persisting the changes.

The code handles both new and existing users correctly and takes care of updating the user's information based on the claims.

---

Line range hint 477-507: The registerNodeForOIDCCallback method registers a new node securely.

The method allocates IPv4 and IPv6 addresses for the node using the IP allocator. It then registers the node in the database using the machine key, user information, expiry, and allocated IP addresses.

It uses a database transaction to ensure that the registration is atomic and consistent. If the registration fails for any reason, it returns an appropriate error.

The code handles the node registration process correctly and takes care of allocating IP addresses and storing the necessary information in the database.

---

Line range hint 510-522: The renderOIDCCallbackTemplate function renders the callback template correctly.

The function takes the OIDC claims as input and executes the OIDC callback template. It passes the user's email and a verb ("Authenticated") as the template data, providing the necessary information for rendering.

If the template rendering fails, it returns an appropriate error. Otherwise, it returns the rendered template content.

The code handles the template rendering process correctly and provides the required data to the template.

hscontrol/db/routes.go (1)

647-647: Approve the change in user identifier comparison.

The change from comparing approvedAlias with node.User.Name to node.User.Username() is an improvement. It ensures that the correct user identifier is used for determining approved aliases, potentially enhancing the accuracy of the auto-approval process for routes.

hscontrol/db/db.go (2)

413-424: LGTM! The new migration step for the User type is a necessary update.

The addition of the migration step with ID 202407191627 is crucial for maintaining data integrity and supporting the updated user management functionality. The AutoMigrate function ensures that the User structure is properly integrated into the database schema.

Please ensure thorough testing of the user management features to confirm a smooth transition and verify that the migration does not introduce any unintended side effects.

---

268-268: Verify the impact of removing the hostname normalization.

The removal of the normalization step when assigning the GivenName property of Node objects simplifies the code. However, please ensure that this change does not introduce inconsistencies or violate any expected hostname formats in the system.

Consider running the following script to analyse the existing GivenName values and confirm that they align with the expected format:

Replace <path_to_db> with the path to your SQLite database file. The script retrieves the GivenName values, formats them as a JSON array, and pipes the output to jq for pretty-printing. Review the output to ensure the values meet the expected hostname format.

hscontrol/auth.go (7)

21-24: LGTM!

The AuthProvider interface provides a clean abstraction for handling authentication, allowing different authentication strategies to be implemented. The method signatures are well-defined and serve their intended purpose.

---

Line range hint 21-244: Simplification of handleRegister function.

The removal of direct machine key handling in several function calls within handleRegister simplifies the function's signature and improves readability. The core logic of the function remains intact, handling various registration scenarios based on the state of the node in the database.

---

171-172: Simplified handleNodeLogOut function signature.

The removal of the machineKey parameter from the handleNodeLogOut function simplifies the function signature without affecting the core functionality of logging out a client and handling ephemeral nodes.

---

179-180: Simplified handleNodeWithValidRegistration function signature.

The removal of the machineKey parameter from the handleNodeWithValidRegistration function simplifies the function signature without affecting the core functionality of handling a client with a valid registration and redirecting them to the /map endpoint.

---

Line range hint 189-191: Simplified handleNodeKeyRefresh function signature.

The removal of the machineKey parameter from the handleNodeKeyRefresh function simplifies the function signature without affecting the core functionality of updating the node's key in the database and responding to the client with the appropriate fields.

---

452-453: Centralized authentication URL generation using AuthProvider interface.

The utilization of the AuthProvider interface to generate the authentication URL in the handleNewNode function centralizes the URL generation logic, making the code cleaner and more maintainable. The AuthURL method of the interface is called with the machine key as a parameter, and the generated URL is set in the response's AuthURL field.

---

677-678: Centralized authentication URL generation using AuthProvider interface.

Similar to the changes in the handleNewNode function, the utilization of the AuthProvider interface to generate the authentication URL in the handleNodeExpiredOrLoggedOut function centralizes the URL generation logic, making the code cleaner and more maintainable. The AuthURL method of the interface is called with the machine key as a parameter, and the generated URL is set in the response's AuthURL field.

hscontrol/db/node.go (4)

335-335: LGTM!

The function update to use GetUserByName instead of GetUser is appropriate and consistent with the naming convention.

---

388-388: LGTM!

The update to use node.User.Username() instead of node.User.Name in the log statement improves encapsulation and maintainability.

---

404-404: LGTM!

The update to use node.User.Username() instead of node.User.Name in the log statement is consistent with the previous change and improves encapsulation.

---

624-625: LGTM!

The updates to the generateGivenName function improve the logic flow and simplify the code:

• Moving the hostname length check to the beginning of the function ensures early validation.
• Removing the hostname normalization step simplifies the logic.
• Using the supplied name directly for trimming improves clarity and reduces unnecessary operations.

The changes enhance the readability and maintainability of the function.

Also applies to: 631-632, 640-640

hscontrol/policy/acls.go (2)

929-929: LGTM!

The string equality check is the correct way to verify if the node's user has the authority to add the tag.

---

953-953: LGTM!

The string equality check is the correct way to filter nodes by user.

hscontrol/app.go (4)

98-98: LGTM!

The addition of the authProvider field enhances modularity by abstracting the authentication mechanism through the AuthProvider interface. This allows for easier integration of different authentication strategies in the future.

---

154-178: LGTM!

The AuthProvider initialization logic in the NewHeadscale function is well-structured and provides a fallback mechanism. It encapsulates the authentication setup based on the configuration, ensuring that the application can function with either OIDC or CLI-based authentication. The assignment of the authProvider field centralizes the authentication provider for the application.

---

444-444: LGTM!

The updated routing logic for the /register/{mkey} endpoint now directly calls the RegisterHandler method of the authProvider. This change centralizes the registration handling logic within the AuthProvider implementation, promoting a more modular and maintainable codebase.

---

446-448: LGTM!

The conditional registration of the OIDC callback endpoint based on the type of authProvider is a good practice. It ensures that the callback is only exposed when OIDC authentication is being used, preventing unnecessary exposure when OIDC is not configured. The direct registration of the callback to the OIDCCallback method of the AuthProviderOIDC instance keeps the OIDC-specific logic encapsulated within the provider, promoting a cleaner separation of concerns.

CHANGELOG.md (6)

7-8: **** The previous review comment is still valid and applicable:

The removal of this configuration option is noted as a breaking change, which could impact existing deployments that rely on this feature for DNS configurations. It's crucial to ensure that this change is clearly communicated to users to prevent potential disruptions.

---

9-11: **** The previous review comment is still valid and applicable:

The removal of the strip_email_domain option and the shift to using the sub claim for user identification are significant. This change enhances security and flexibility in user management but requires careful migration planning for existing systems to adapt without issues.

---

12-14: **** The previous review comment is still valid and applicable:

Adding fields such as username, display name, profile picture URL, and email enhances the user experience by providing more detailed user profiles. These fields also prepare the system for future enhancements, such as API/CLI access for non-OIDC users. It's important to ensure that these fields are handled securely, especially concerning data privacy and potential exposure through public interfaces.

---

3-4: **** The addition of the "Next" section is a good practice.

It provides a clear separation for upcoming changes or features, helping users and contributors understand what to expect in future releases.

---

5-6: **** The addition of the "BREAKING" subsection is crucial.

It highlights breaking changes in the upcoming release, helping users and contributors prepare for potential compatibility issues and plan their upgrades accordingly.

---

15-15: **** The addition of the new release section for version 0.23.0 follows the existing changelog format.

It provides a clear separation for the changes introduced in the specific release version.

hscontrol/types/config.go (6)

74-74: LGTM!

The removal of the unused DNSUserNameInMagicDNS field from the Config struct simplifies the DNS configuration and aligns with the deprecation of the use_username_in_magic_dns configuration key.

---

92-96: LGTM!

The removal of the unused UserNameInMagicDNS field from the DNSConfig struct simplifies the DNS configuration and aligns with the deprecation of the use_username_in_magic_dns configuration key. The addition of mapstructure tags ensures proper mapping of configuration values to the struct fields.

---

320-323: LGTM!

Registering fatal errors for the deprecated configuration keys oidc.strip_email_domain, dns.use_username_in_musername_in_magic_dns, and dns_config.use_username_in_musername_in_magic_dns ensures that the application fails to start if these keys are still present in the configuration. This enforces the removal of the deprecated keys and encourages users to update their configuration.

---

326-334: LGTM!

The introduced loop checks for the presence of removed configuration keys and logs a fatal error if any of them are set in the configuration. The fatal error message provides clear instructions to the user to remove the key from the configuration file. This enforces the removal of the removed keys and encourages users to update their configuration.

---

790-795: LGTM!

Returning an error for an invalid prefixes.allocation configuration value ensures that the application fails to start with an invalid configuration. The error message provides clear information about the invalid value and the allowed options (sequential and random), helping users identify and correct the misconfiguration.

---

829-833: LGTM!

Returning an error when server_url contains base_domain prevents a configuration that would cause the headscale server and embedded DERP to become unreachable from the Tailscale node. The error message provides a clear explanation of the issue and its impact, helping users identify and correct the misconfiguration.

hscontrol/policy/acls_test.go (2)

651-651: Corrected the expected output for the "Expand emails in group" test case.

The change fixes the expected result to contain the full email addresses "[email protected]" and "[email protected]" instead of the incorrect values. This aligns the test with the intended behaviour of returning email addresses without any alterations.

---

344-344: Added a new test case for IPv6 support.

The "ipv6" test case is a valuable addition that verifies the correct handling of IPv6 addresses in the ACL policy. It ensures that the generated filter rules include both IPv4 and IPv6 addresses as expected. This improves the test coverage for IPv6 functionality.

Files that changed from the base of the PR and between e66d149 and 45bc39c.

• go.sum is excluded by !**/*.sum

• flake.nix (1 hunks)
• go.mod (1 hunks)
• hscontrol/app.go (6 hunks)
• hscontrol/auth.go (5 hunks)
• hscontrol/db/db.go (6 hunks)
• hscontrol/db/db_test.go (3 hunks)
• hscontrol/db/node.go (6 hunks)
• hscontrol/db/routes_test.go (1 hunks)
• hscontrol/db/suite_test.go (1 hunks)
• hscontrol/grpcv1.go (4 hunks)
• hscontrol/oidc.go (9 hunks)
• hscontrol/types/node_test.go (3 hunks)

• flake.nix
• hscontrol/db/db.go
• hscontrol/grpcv1.go

hscontrol/db/suite_test.go (1)

62-62: Clarify the purpose and impact of emptyCache().

The newTestDB function has been updated to include a new argument emptyCache(). However, the purpose and impact of this function are not clear from the provided context.

Please provide more information about what emptyCache() does and how it affects the database setup process. This will help ensure that the change is properly reviewed and any potential issues are identified.

hscontrol/db/db_test.go (3)

10-10: Looks good!

The import of the time package is appropriate as it is a standard library package in Go that provides functionality for measuring and displaying time.

---

17-17: Import looks appropriate.

The import of the zgo.at/zcache/v2 package is appropriate as it likely provides caching functionality that is used in the code. The import path also suggests it is version 2 of the package.

---

127-127: Appropriate usage of caching.

The changes to the NewHeadscaleDatabase function call are appropriate:

• The empty string argument is likely a placeholder for a parameter that is not relevant in this test context.
• The emptyCache() function creates and returns a new instance of zcache.Cache with specific time durations for expiration and cleanup.
• The cache is likely used by the HSDatabase for performance optimization.

hscontrol/types/node_test.go (5)

Line range hint 17-103: LGTM!

The test cases for Test_NodeCanAccess are well-structured, cover a good range of scenarios, and follow best practices such as using the cmp package for comparisons and helper functions for improved readability.

---

Line range hint 105-206: Looks good!

The changes to the test cases in TestNodeFQDN streamline the testing process and focus on important edge cases related to username length and DNS configurations. The addition of the new test case for handling long usernames is a valuable improvement.

---

Line range hint 208-298:

---

Line range hint 300-374:

---

Line range hint 376-420:

go.mod (2)

210-210: Verify the necessity and compatibility of the added dependency.

Please ensure that the added indirect dependency zgo.at/zcache at version v1.2.0 is necessary for the project and compatible with the project's license and other dependencies.

---

211-211: Verify the necessity and compatibility of the added dependency and manage potential version conflicts.

Please ensure that the added indirect dependency zgo.at/zcache/v2 at version v2.1.0 is necessary for the project and compatible with the project's license and other dependencies.

Also, note that the zgo.at/zcache dependency is added for both v1.2.0 and v2.1.0 versions. Please ensure that this does not lead to any version conflicts in the project.

hscontrol/oidc.go (6)

49-103: Excellent refactoring to improve modularity and clarity!

The introduction of the AuthProviderOIDC struct and the corresponding constructor function NewAuthProviderOIDC enhances the code structure by encapsulating related components and initializing the OIDC provider and OAuth2 configuration in a clear and concise manner. The error handling in the constructor function is also appropriate, returning an error if the OIDC provider creation fails.

These changes promote better modularity, readability, and maintainability of the codebase.

---

Line range hint 122-173: LGTM! The changes align with the refactoring.

The updates to the RegisterHandler method, making it a method of the AuthProviderOIDC struct, are in line with the overall refactoring of moving OIDC-related functionality into the AuthProviderOIDC struct. The error handling is appropriate, returning HTTP errors for invalid machine key or internal server errors.

The caching of the node key using the registrationCache is a good approach to retrieve it later in the callback.

---

Line range hint 192-269: Great job with the modular structure and error handling!

The updates to the OIDCCallback method, making it a method of the AuthProviderOIDC struct, align with the overall refactoring. The error handling is thorough, returning appropriate HTTP errors for various failure scenarios.

The method delegates the extraction of the ID token, validation of allowed domains/groups/users, and creation/update of the user to separate methods, promoting a modular and readable structure. The registration of the node is handled by the registerNodeForOIDCCallback method, which is a good separation of concerns.

Overall, the changes enhance the clarity and maintainability of the code.

---

271-307: Improved readability and error handling!

The introduction of the extractCodeAndStateParamFromRequest function as a helper function encapsulates the extraction logic, improving the readability of the code.

The changes to the extractIDToken method signature, making it a method of the AuthProviderOIDC struct, align with the overall refactoring. The error handling in both functions is appropriate, returning errors for missing parameters or failed token exchange/verification.

These changes enhance the clarity and robustness of the code.

---

Line range hint 309-359: Clear structure and modularity in validation functions!

The validateOIDCAllowedDomains, validateOIDCAllowedGroups, and validateOIDCAllowedUsers functions are well-structured and follow a clear logic for checking the allowed domains, groups, and users. The error handling is appropriate, returning specific errors for each validation failure scenario.

The separation of the validation logic into distinct functions promotes code reusability and modularity, making the code easier to understand and maintain.

Great job with the validation functions!

---

442-498: Improved user management and modular node registration!

The createOrUpdateUserFromClaim method handles the logic for creating or updating a user based on the OIDC claims, enhancing the user management process. The inclusion of a check for legacy support, looking up the user by username if not found by OIDC identifier, is a good transitional approach. The error handling in the method is appropriate, propagating any errors that occur during user retrieval or saving.

The registerNodeForOIDCCallback method encapsulates the node registration logic, promoting a modular and focused design. It handles the allocation of IPv4 and IPv6 addresses and registers the node with the appropriate details.

These changes improve the overall structure and maintainability of the codebase.

hscontrol/auth.go (5)

21-24: Excellent addition of the AuthProvider interface!

Defining the AuthProvider interface is a great step towards making the authentication mechanism more modular and flexible. It allows different authentication strategies to be easily plugged in and swapped out as needed. This change promotes a cleaner separation of concerns and improves the overall design of the authentication system.

---

170-170: Good simplification of the handleNodeLogOut function signature.

Removing the machineKey parameter from the handleNodeLogOut function is a positive change. It simplifies the function signature and improves readability by reducing the number of parameters. This suggests that the node parameter alone provides sufficient information for handling the node logout process. Well done on this simplification!

---

178-178: Another good simplification of the function signature.

Removing the machineKey parameter from the handleNodeWithValidRegistration function is consistent with the previous change and further improves the codebase. It makes the function signature cleaner and more focused. The node parameter seems to provide all the required information for handling a node with a valid registration. This change enhances readability and maintainability. Great job on this simplification as well!

---

Line range hint 389-399: Proper use of database transaction and error handling.

The code segment demonstrates the correct usage of a database transaction using the db.Write function. Executing the db.UsePreAuthKey operation within a transaction ensures data integrity and consistency. If any error occurs during the transaction, it is properly handled, and an appropriate error response is returned. This helps maintain the reliability and stability of the system. Well done on implementing the transaction and error handling correctly!

---

450-450: Effective use of the AuthProvider interface.

Assigning the value returned by h.authProvider.AuthURL(machineKey) to resp.AuthURL is an excellent way to leverage the AuthProvider interface. It allows the authentication URL to be generated dynamically based on the specific authProvider implementation and the provided machineKey. This change promotes modularity and flexibility in the authentication process. It enables different authentication providers to be easily integrated without modifying the core logic. Great job on utilizing the AuthProvider interface effectively!

hscontrol/db/node.go (2)

Line range hint 315-368: LGTM!

The RegisterNodeFromAuthCallback method looks good. It properly handles node registration from an API/CLI or auth callback, updating the node's details if found in the registration cache or returning an appropriate error if not found. The logic flow is clear and error handling is robust.

---

Line range hint 379-426: LGTM!

The RegisterNode function looks good. It properly handles registering a new node, distinguishing between nodes that already have IP addresses (just saving to update expiry and node key) and those that don't (ensuring a unique given name and setting IP addresses before saving). The logic flow is clear, error handling is robust, and logging is appropriate.

hscontrol/db/routes_test.go (1)

335-335: LGTM! The addition of the emptyCache() parameter is a good practice.

Passing emptyCache() to NewHeadscaleDatabase ensures that each test runs with a clean cache state. This prevents potential side effects from previous tests and makes the tests more reliable and independent of each other.

hscontrol/app.go (2)

Line range hint 123-179: Excellent work on introducing the AuthProvider abstraction! 🌟

The changes in the NewHeadscale function significantly improve the flexibility and modularity of the authentication mechanism. By introducing the AuthProvider interface, the code now supports different authentication strategies, such as the default AuthProviderWeb and the optional AuthProviderOIDC. This abstraction allows for easier integration of new authentication methods in the future.

Moreover, the OIDC provider initialisation is now handled gracefully. If the OIDC initialisation fails, the application can either return an error or fall back to the default authentication provider based on the configuration. This enhances the robustness of the authentication process.

Additionally, moving the registration cache initialisation within this function improves encapsulation and keeps the related logic together.

Overall, these changes demonstrate a well-structured and modular approach to handling authentication in the Headscale application. Great job! 👍

---

445-449: Great refactoring of the registration route handling! 👏

The modifications in the createRouter function demonstrate a cleaner and more modular approach to handling registration requests. By delegating the registration handling to the RegisterHandler method of the authProvider, the code achieves better separation of concerns and promotes consistency across different authentication strategies.

Furthermore, the conditional registration of the OIDC callback route based on the type of authProvider is a nice touch. It ensures that the callback route is only available when using the OIDC authentication provider, preventing unnecessary route registration and improving code clarity.

These changes enhance the overall structure and maintainability of the authentication flow in the Headscale application. Well done! 👍

Files that changed from the base of the PR and between 45bc39c and c9e6b61.

• hscontrol/db/node.go (4 hunks)
• hscontrol/db/users.go (6 hunks)
• hscontrol/grpcv1.go (4 hunks)
• hscontrol/oidc.go (9 hunks)
• hscontrol/types/users.go (5 hunks)

• hscontrol/db/node.go
• hscontrol/db/users.go
• hscontrol/grpcv1.go
• hscontrol/types/users.go

hscontrol/oidc.go (9)

49-58: Excellent work on encapsulating OIDC-related fields in a dedicated struct.

This change enhances code organization and modularity by grouping related fields together. It will make the code more maintainable and easier to understand.

---

61-103: Great job on implementing the constructor function for AuthProviderOIDC.

The function properly initializes all the necessary fields of the struct using the provided dependencies. It encapsulates the initialization logic in a single place, making it easier to create instances of AuthProviderOIDC correctly.

---

105-109: The AuthURL method looks good.

It correctly generates the authentication URL for a given machine key by appending it to the server URL with the appropriate path.

---

112-118: The determineTokenExpiration method is implemented correctly.

It properly determines the token expiration time based on the configuration settings, either using the expiry from the token or calculating a new expiry using the configured duration.

---

Line range hint 122-173: The RegisterHandler method is implemented robustly.

It handles the OIDC registration process effectively by:

• Extracting and validating the machine key from the request URL.
• Generating a random state and storing the machine key in the registration cache.
• Constructing the authorization URL with the necessary parameters and redirecting the user to the OIDC provider.
• Handling errors appropriately and returning meaningful error responses.

Well done!

---

271-282: The extractCodeAndStateParamFromRequest function is implemented correctly.

It properly extracts the code and state parameters from the request URL query and returns an error if either parameter is empty. The function is simple and serves its purpose well.

---

284-307: The extractIDToken method is implemented securely.

It handles the extraction and verification of the ID token from the OAuth2 token correctly by:

• Exchanging the code for an OAuth2 token using the proper configuration.
• Extracting the raw ID token from the OAuth2 token extras.
• Verifying the ID token using the OIDC provider's verifier and the client ID.
• Handling errors appropriately and returning meaningful error messages.

The method ensures the integrity and validity of the ID token before proceeding further.

---

442-473: The createOrUpdateUserFromClaim method is implemented effectively.

It handles the creation or update of a user based on the OIDC claims correctly by:

• Attempting to retrieve the user by the OIDC identifier first.
• Falling back to username lookup for legacy compatibility if the user is not found by the OIDC identifier.
• Creating a new empty user if the user is still not found.
• Updating the user fields based on the claims and saving the user to the database.
• Handling errors appropriately and returning meaningful error messages.

The method ensures that the user is properly created or updated based on the available information in the OIDC claims.

---

Line range hint 361-439: The validateNodeForOIDCCallback method handles node validation and reauthentication, but there is room for improvement.

The method is currently handling multiple responsibilities, including node validation, reauthentication, and rendering the HTML template. Consider refactoring the method to separate these concerns and improve readability and maintainability. For example:

• Extract the node lookup and expiry update logic into a separate method.
• Move the notification logic to a dedicated method.
• Extract the HTML template rendering to a separate function.

This will make the code more modular and easier to understand and maintain.

Additionally, please verify the impact of the changes made to the node validation and reauthentication process on the overall codebase. Ensure that the changes do not introduce any unintended side effects or break existing functionality.

Run the following script to check for potential issues:

Review the results and confirm that the changes are compatible with the existing codebase and do not introduce any regressions.

Verification successful

---

Refactoring suggestion for validateNodeForOIDCCallback is valid and can be extended further

The review comment's suggestion to refactor the validateNodeForOIDCCallback method is indeed valid and can be implemented without introducing unintended side effects. Based on the analysis of the codebase, we can extend the refactoring suggestion to improve code reuse and maintainability:

1. Extract the node lookup and expiry update logic into a separate method, as it's used in multiple places across the codebase.
2. Create a dedicated method for sending notifications, which can be reused in other parts of the codebase that perform similar operations.
3. Move the HTML template rendering to a separate function, as suggested in the original review.

These changes will not only improve the readability and maintainability of the validateNodeForOIDCCallback method but also enhance the overall structure of the codebase by promoting code reuse.

To implement these changes:

• Create utility functions for node lookup, expiry update, and notifications in a separate file (e.g., node_utils.go).
• Update all occurrences of these operations throughout the codebase to use the new utility functions.
• Refactor the validateNodeForOIDCCallback method to use these utility functions and separate the HTML rendering logic.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for usages of the validateNodeForOIDCCallback method
rg --type go -A 5 $'validateNodeForOIDCCallback'

# Check for potential issues related to node expiry and notifications
rg --type go -A 5 $'NodeSetExpiry|NotifyByNodeID|NotifyWithIgnore'

Length of output: 7024